-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add type annotations to torch.nn.modules.module #49045
Conversation
💊 CI failures summary and remediationsAs of commit b9382f2 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. This comment has been revised 33 times. |
Codecov Report
@@ Coverage Diff @@
## master #49045 +/- ##
==========================================
- Coverage 80.72% 80.72% -0.01%
==========================================
Files 1904 1904
Lines 206764 206764
==========================================
- Hits 166908 166907 -1
- Misses 39856 39857 +1 |
@guilhermeleobas could you also add comments here about the ignores you added? That helps both with review (reproducing the failures and using |
Also, in your next commit message, can you add that it closes gh-48640? |
Hi @rgommers, thanks for the review. I've addressed your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thanks @guilhermeleobas.
CI notifications seem stuck, but CI is done - see https://app.circleci.com/pipelines/github/pytorch/pytorch?branch=pull%2F49045. Failures are unrelated.
torch/fx/symbolic_trace.py
Outdated
# Type ignore here is required because "n" and "p" have different types | ||
# a few lines above | ||
for n, p in self.root.named_buffers(): # type: ignore[assignment] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why cant we just use different variable names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that works too
torch/nn/modules/module.py
Outdated
load = None # break load->load reference cycle | ||
# instruction "load = None" => break load->load reference cycle | ||
# type ignore[...] is required because mypy isn't happy with the redef of "load" | ||
load = None # type: ignore[assignment] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not python expert. but cant we use del load
to remove the function reference itself? (after all the load has been done, yes?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Do not merge this PR until one checks if the annotations introduce any regression. See: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picky back on this PR - i dont think there's any additional item that needs to be test with JIT-ability for this change. am I missing any? @malfet
@@ -279,7 +279,7 @@ def parameters(self, recurse: bool = True) -> Iterator[Parameter]: | |||
|
|||
def named_parameters( # type: ignore[return] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we remove this type ignore as well since we fixed the return Parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
torch/distributed/nn/api/remote_module.py:280: error: Missing return statement [return]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. Got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. I dont see any additional JIT test required.
actually please rebase master --> seems like merge conflict is preventing fb internal CI to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@walterddr merged this pull request in 5f8e1a1. |
Fixes #49044